Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: consider celery imports from common settings plus env tokens #29739

Merged
merged 1 commit into from Apr 14, 2022
Merged

refactor: consider celery imports from common settings plus env tokens #29739

merged 1 commit into from Apr 14, 2022

Conversation

MaferMazu
Copy link
Contributor

@MaferMazu MaferMazu commented Jan 10, 2022 •
edited

Description

This allows to add extra celery imports from environment variables.

Other info

For this PR I take the suggestion to change the tuple for a list and to use the name CELERY_EXTRA_IMPORTS from this comment

For more information: Discuss Post
Related PR: #30222

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 10, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @MaferMazu! I've created OSPR-6369 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@natabene
Copy link
Contributor

@MaferMazu Thank you for your contribution. Please let me know once this is ready for our review.

@MaferMazu
Copy link
Contributor Author

@natabene Thank you for being so attentive. Is ready for review :D

@natabene
Copy link
Contributor

@MaferMazu Great, let's first see how checks turn out.

@MaferMazu MaferMazu force-pushed the mfmz/celery-import-upgrade branch 2 times, most recently from 7a1f97c to 522f6f2 Compare January 18, 2022 22:31
@jinder1s
Copy link
Contributor

@MaferMazu Hi, thanks for this commit. I'm trying to understand what this setting does. Can you give me more details about how this setting (CELERY_IMPORTS) would be used? If you could point me to code that would use this setting, that would be great.

@MaferMazu
Copy link
Contributor Author

I'm interested in having some celery imports in environment variables, and I propose this PR so that my environment variables don't overwrite changes like these: https://github.com/openedx/edx-platform/pull/29173/files

cc @jinder1s

jinder1s
jinder1s previously approved these changes Jan 26, 2022
View reviewed changes
@jinder1s jinder1s self-requested a review January 26, 2022 16:01
@jinder1s jinder1s dismissed their stale review January 26, 2022 16:02

Accidental approval.

jinder1s
jinder1s reviewed Jan 26, 2022
View reviewed changes
lms/envs/production.py Outdated Show resolved Hide resolved
jinder1s
jinder1s approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@jinder1s jinder1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the later approval. Thanks for this PR!

@MaferMazu
Copy link
Contributor Author

Thanks for your review :D, i am going to rebase.

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Mar 15, 2022 •
edited

Hi there! @MaferMazu, can you rebase and fix the failing tests? Thanks!

Also, you can change your commit to be more descriptive. Maybe try:
refactor: consider celery imports from common settings plus env tokens

@natabene
Copy link
Contributor

natabene commented Apr 1, 2022

@MaferMazu Just checking if you are still interested in pursuing this.

@MaferMazu
Copy link
Contributor Author

Excuse me @mariajgrimaldi and @natabene
Yes, I want to pursue this.
I am going to take the recommendations of @mariajgrimaldi and it's ready.

robrap
robrap reviewed Apr 5, 2022
View reviewed changes
lms/envs/production.py Outdated Show resolved Hide resolved
@MaferMazu MaferMazu changed the title fix: celery import upgrade refactor: consider celery imports from common settings plus env tokens Apr 12, 2022
robrap
robrap approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks!

@robrap robrap merged commit a099d9a into openedx:master Apr 14, 2022
@openedx-webhooks
Copy link

@MaferMazu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been rolled back from the production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been rolled back from the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robrap robrap robrap approved these changes

@jinder1s jinder1s jinder1s approved these changes

Assignees
No one assigned
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@MaferMazu @openedx-webhooks @natabene @jinder1s @mariajgrimaldi @edx-pipeline-bot @robrap